Skip to content

Conversation

@gr5
Copy link
Collaborator

@gr5 gr5 commented Jan 8, 2026

I don't understand what I did but it seems to work. I don't understand the calculations of xDel and hDelLimit and so on. Well I get lambda - that's the wavelength. But it seems to work perfectly so I think I did it right.

All the code is cut and pasted except as you can see I changed an object from class QwtPlotCurve to ProfileCurve (ProfileCurve has ability to show slope and QwtPlotCurve does not).

Anyway I tested it against the non average and it seems to show slope in the same steepness.

@githubdoe
Copy link
Owner

Yes the new profile curve class knows how to plot the slope error.

xDel is the x dimension over which the slope will be calculated. hDelLimit is how high the y can vary before the slope exceeds the limit set by the user.

Copy link
Owner

@githubdoe githubdoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why you added the code to set the slope settings. I would think they were already set.

@github-actions
Copy link

github-actions bot commented Jan 8, 2026

🚀 New build available for commit 46f2c3d
Download installer here

@gr5
Copy link
Collaborator Author

gr5 commented Jan 8, 2026

I don't know why you added the code to set the slope settings. I would think they were already set.

No I had to set the slope settings. Because just above is that "new" which created an instance of class ProfileCurve and so I had to set the boolean m_showSlopeError with setSlopeSettings(). The boolean isn't a static. It's a normal member variable.

If you look at the code in entirety there is a section that creates the single profiles, the 16 profiles and the average profiles and each section creates their own instances and each section of code needs to set the slope flag thing.

Copy link
Owner

@githubdoe githubdoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my mistake I was in a hurry and did not see what was really getting set.

left.append(right);

QwtPlotCurve *cprofileavg = new QwtPlotCurve( name + " avg");
ProfileCurve *cprofileavg = new ProfileCurve( name + " avg");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a memory leak here.
A new with no delete and no Qt object with parent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting point. I'll look at this today.

@gr5
Copy link
Collaborator Author

gr5 commented Jan 9, 2026

I'm going to merge this now and probably create a release (8.3.2) and if I agree there is a memory leak (quite likely but not 100% definite) I will create an "issue" and also a PR.

Actually it's pretty easy I suppose to create a destructor and put a break point on it. When creating all new profiles the old ones need to get deleted.

@gr5 gr5 merged commit 034609d into master Jan 9, 2026
14 checks passed
@gr5
Copy link
Collaborator Author

gr5 commented Jan 9, 2026

I think it's fine (no leaks). These 2 lines of code have a second default parameter that deletes everything. I think. So the items created are "attached" before plotting them. Then the next time you change what profiles you create these are called at the start:

    m_plot->detachItems(QwtPlotItem::Rtti_PlotItem);
    m_plot->detachItems(QwtPlotItem::Rtti_PlotTextLabel);

I'll create a destructor to be sure. But by default those detachItems() delete everything that was attached earlier. I hope.

@gr5
Copy link
Collaborator Author

gr5 commented Jan 9, 2026

Confirmed! No memory leak.

@atsju
Copy link
Collaborator

atsju commented Jan 9, 2026

During review I though this was a new variable so I did miss the existing attach.
thanks for double checking about the potential leak. Sorry for false positive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants